Assert logs and run test suites conditionally#143
Conversation
kraenhansen
added
CI
|
kraenhansen
changed the title
kraenhansen
force-pushed
the
kh/assert-example-logs
branch
from
88a424b to
6ca4b54
Compare
kraenhansen
force-pushed
the
main
branch
4 times, most recently
from
90dd405 to
8dac805
Compare
kraenhansen
force-pushed
the
kh/assert-example-logs
branch
from
6ca4b54 to
60677f4
Compare
| } | ||
| } | ||
|
|
||
| export const suites: Record<string, Record<string, () => void>> = { |
There was a problem hiding this comment.
I wasn't clear how/when this test file would be run, nor whether it'd be in Node.js or React Native. May be worth adding some brief docs in packages/node-addon-examples/README.md and apps/test-app/README.md.
There was a problem hiding this comment.
I revisited the packages/node-addon-examples/README.md.
| allTests = false, | ||
| nodeAddonExamples = allTests, | ||
| ferricExample = allTests, |
There was a problem hiding this comment.
So by default, both nodeAddonExamples and ferricExample resolve to false and so both the "Node Addon Examples" and "ferric-example" tests get skipped? Is that the intention?
There was a problem hiding this comment.
I might still be debating this with myself 😊
On one hand it might be confusing to skip so many tests by default and on the other it does leave a potential to cut down on the time pre-building while bootstrapping and complexity (having to have the Rust toolchain installed) when someone wants to contribute to the project.
Looking back at this (I wrote it some weeks back), the optimization does seem a bit premature.
There was a problem hiding this comment.
In my (though not strongly held)) opinion we could either enable all tests by default or skip only ferric-example since it requires additional Rust setup.
kraenhansen
force-pushed
the
kh/assert-example-logs
branch
from
60677f4 to
8afc47e
Compare
|
Great job, I liked introduced changes and converting example into real tests. |
|
Feel free to merge if you're happy – I am unlikely to have time to look (and understand) much deeper! |
kraenhansen
force-pushed
the
kh/assert-example-logs
branch
from
8afc47e to
34408c3
Compare
3 participants
Merging this PR will:
This also fixes #141.